Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump GTDBTk to v2.4.0 #664

Merged
merged 4 commits into from
Sep 10, 2024
Merged

Bump GTDBTk to v2.4.0 #664

merged 4 commits into from
Sep 10, 2024

Conversation

dialvarezs
Copy link
Contributor

Update GTDBTk module to latest version, and update summary_gtdbtk.py to hande column name changes.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/mag branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@dialvarezs dialvarezs changed the title Update GTDBTk to v2.4.0 Bump GTDBTk to v2.4.0 Sep 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 8, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 080eac6

+| ✅ 306 tests passed       |+
#| ❔   2 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Remove this line if you don't need a FASTA file [TODO: try and test using for --host_fasta and --host_genome]
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-09 09:01:29

@jfy133
Copy link
Member

jfy133 commented Sep 9, 2024

Thank you very much @dialvarezs ! A very nice thing to come abck from weekend too 😎

I',m going to run a test on a HPC (as we can't test GTDB on GitHub due to the massive database file), and if that works I will merge this in :)

@jfy133
Copy link
Member

jfy133 commented Sep 9, 2024

OK took a long while to get it running on the HPC (it's a new one which I completely forgot about 😅 ), and had to manually download the GTDB database as it's an offline cluster, then insufficient memory, and a bunch of other things...

IT's running now but waiting for a 256GB node to be allocated to me...

@dialvarezs
Copy link
Contributor Author

Don't worry, and thanks for the review! I forgot to update the db parameter 😅.
I've run MAG with this modification a few times over the last month in our HPC, and so far, everything has worked without any issues.

@jfy133
Copy link
Member

jfy133 commented Sep 9, 2024

Oh nice! What amount of memory were you generally having to use to get it to run? Given the database is like 40gb larger than the other I wonder if we need to bump the minimum amount

@dialvarezs
Copy link
Contributor Author

Good question. I'm using the default 128GB, and until now I have seen no failed jobs. Now, looking into it, Slurm reports a MaxRSS of around 10GB, while Nextflow reports about 300GB. I think this is because pplacer uses RAM as disk cache, and switches to scratch space if memory is not enough.

@jfy133
Copy link
Member

jfy133 commented Sep 10, 2024

OK cool, thanks for that! I've discovered a bug ion the nf-core module (because our test data MAGs doesn't get hits in the GTDB database funnily enough), but once I've fixed that I'll update the module in your PR, run my test again, and then I thin kwe are pretty much good to go :)

jfy133
jfy133 approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM And works for me too, thank you @dialvarezs ! Impactful first contribution 💪

@jfy133 jfy133 merged commit 4dd9519 into nf-core:dev Sep 10, 2024
15 checks passed
@jfy133
Copy link
Member

jfy133 commented Sep 13, 2024

Whelp I forgot to push the fixed module 😆

@jfy133 jfy133 mentioned this pull request Oct 2, 2024
Merged
11 tasks
@amizeranschi amizeranschi mentioned this pull request Oct 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dialvarezs @jfy133